docs/PR_OLLAMA_INTEGRATION.md (2)

60-62: Add language identifier to fenced code block.

The fallback logic code block is missing a language identifier. Consider adding text or mermaid for proper rendering.

🔎 Proposed fix

-```
+```text
 Ollama (local) → Claude → Kimi K2 → Error

</details>

---

`306-307`: **Format URLs as markdown links.**

Bare URLs should be properly formatted as markdown links for better readability and accessibility.



<details>
<summary>🔎 Proposed fix</summary>

```diff
-- **Discord:** https://discord.gg/uCqHvxjU83
-- **Email:** mike@cortexlinux.com
+- **Discord:** [https://discord.gg/uCqHvxjU83](https://discord.gg/uCqHvxjU83)
+- **Email:** [mike@cortexlinux.com](mailto:mike@cortexlinux.com)

docs/OLLAMA_INTEGRATION.md (1)

42-42: Specify language for code block.

The bash command should be wrapped in a code block with language specification for proper syntax highlighting.

🔎 Proposed fix

+```bash
 cortex install nginx
+```

cortex/llm/interpreter.py (1)

144-180: Verify error message and exception type consistency.

The refactored _call_ollama method has good integration with OllamaProvider, but there's a minor inconsistency:

Line 159 error message says "No Ollama models available. Run: ollama pull llama3:8b", but the code now uses codellama:7b as the default model (line 65). The error message should suggest pulling the default model for consistency.

🔎 Proposed fix

             model = ollama.ensure_model_available()
             if not model:
-                raise RuntimeError("No Ollama models available. Run: ollama pull llama3:8b")
+                raise RuntimeError("No Ollama models available. Run: ollama pull codellama:7b")

examples/ollama_demo.py (1)

21-22: Relative path assumption may fail in different execution contexts.

Line 22 uses sys.path.insert(0, '..') to add the parent directory to the Python path. This assumes the script is always run from the examples/ directory, which may not hold true if:

• The script is run from the project root
• The script is executed via absolute path from another directory
• The script is installed as a package

🔎 Proposed fix

Use __file__ to determine the script's location reliably:

+import os
 import sys
 import time
 from rich.console import Console
 from rich.table import Table
 from rich.panel import Panel
 
 # Add parent directory to path
-sys.path.insert(0, '..')
+script_dir = os.path.dirname(os.path.abspath(__file__))
+parent_dir = os.path.dirname(script_dir)
+sys.path.insert(0, parent_dir)

scripts/setup_ollama.py (2)

40-76: Security consideration: Executing downloaded script from external source.

Lines 43-61 download and execute a shell script from https://ollama.com/install.sh. While this is the official Ollama installation method, it presents security risks:

1. No integrity verification: The script is executed without checksum validation
2. HTTPS trust: Relies entirely on HTTPS/TLS for authenticity
3. Arbitrary code execution: Downloaded content is piped to sh -c

This is acceptable since:

• It's the official Ollama installation method documented by Ollama
• Users can skip with CORTEX_SKIP_OLLAMA_SETUP=1
• Error handling prevents installation failures from blocking Cortex setup

However, consider documenting this security trade-off in the code comments.

Suggested documentation addition

     logger.info("📦 Installing Ollama for local LLM support...")
     logger.info("   This enables privacy-first, offline package management")
+    
+    # Security note: This downloads and executes the official Ollama install script.
+    # Users can skip this by setting CORTEX_SKIP_OLLAMA_SETUP=1 environment variable.
+    # See: https://github.com/ollama/ollama for installation verification.
     
     try:

---

100-102: Fixed sleep duration may be insufficient or excessive.

Line 101 uses time.sleep(2) after starting the Ollama service. This arbitrary 2-second wait may:

• Be insufficient on slower systems (service not ready yet)
• Be excessive on fast systems (unnecessary delay)

🔎 Proposed fix

Implement a retry loop with timeout instead of fixed sleep:

         # Give it a moment to start
-        time.sleep(2)
-        logger.info("✅ Ollama service started")
-        return True
+        # Poll for service readiness
+        max_attempts = 10
+        for attempt in range(max_attempts):
+            time.sleep(1)
+            try:
+                import requests
+                response = requests.get("http://localhost:11434/api/tags", timeout=2)
+                if response.status_code == 200:
+                    logger.info("✅ Ollama service started")
+                    return True
+            except Exception:
+                pass
+        
+        logger.warning("⚠️  Service started but not responding yet")
+        return True  # Don't fail - service may still be starting

cortex/llm_router.py (2)

388-390: Token estimation is imprecise.

The word-count-based estimation (len(content.split()) * 1.3) is a rough approximation. Consider using Ollama's actual token count from the response if available, or document this limitation.

🔎 Proposed improvement

-        # Ollama doesn't provide token counts in the same way
-        # Estimate based on response length
-        tokens_used = len(content.split()) * 1.3  # Rough estimate
+        # Use Ollama's token counts if available, otherwise estimate
+        eval_count = response_data.get("eval_count", 0)
+        prompt_eval_count = response_data.get("prompt_eval_count", 0)
+        if eval_count or prompt_eval_count:
+            tokens_used = eval_count + prompt_eval_count
+        else:
+            # Fallback: estimate based on response length
+            tokens_used = int(len(content.split()) * 1.3)

---

319-354: Fallback logic could recurse indefinitely in edge cases.

If a provider fails repeatedly and fallback keeps selecting the same failing provider chain, this could cause deep recursion. The current logic mitigates this by using force_provider, but consider adding a recursion guard.

🔎 Proposed improvement

Add a _fallback_depth parameter to prevent unbounded recursion:

     def complete(
         self,
         messages: list[dict[str, str]],
         task_type: TaskType = TaskType.USER_CHAT,
         force_provider: LLMProvider | None = None,
         temperature: float = 0.7,
         max_tokens: int = 4096,
         tools: list[dict] | None = None,
+        _fallback_depth: int = 0,
     ) -> LLMResponse:

Then guard before recursing:

                 if fallback_provider:
+                    if _fallback_depth >= 2:
+                        raise RuntimeError("Max fallback depth exceeded")
                     logger.info(f"🔄 Attempting fallback to {fallback_provider.value}")

                     return self.complete(
                         messages=messages,
                         task_type=task_type,
                         force_provider=fallback_provider,
                         temperature=temperature,
                         max_tokens=max_tokens,
                         tools=tools,
+                        _fallback_depth=_fallback_depth + 1,
                     )

cortex/providers/ollama_provider.py (2)

263-285: Model pull timeout may be insufficient for large models.

The pull_model() method uses self.timeout (default 300 seconds) for downloading models. Models like deepseek-coder-v2:16b (10GB+) may require significantly longer on slower connections.

🔎 Proposed fix

     def pull_model(self, model_name: str) -> bool:
         """
         Pull a model from Ollama registry.
         
         Args:
             model_name: Name of model to pull
             
         Returns:
             True if successful, False otherwise
         """
         logger.info(f"📥 Pulling model: {model_name}")
         
         try:
             response = requests.post(
                 f"{self.base_url}/api/pull",
                 json={"name": model_name},
                 stream=True,
-                timeout=self.timeout
+                timeout=None  # No timeout for model downloads
             )

Alternatively, add a separate pull_timeout parameter to __init__.

---

374-398: Prompt format may not be optimal for all models.

The simple System:/User:/Assistant: format works for basic models but may not be optimal for instruction-tuned models that expect specific prompt templates (e.g., Llama 3's <|begin_of_text|> format, Mistral's [INST] format). Consider using Ollama's native chat API (/api/chat) instead.

🔎 Proposed improvement

     def complete(
         self,
         messages: list[dict[str, str]],
         model: str | None = None,
         temperature: float = 0.7,
         max_tokens: int = 4096,
         stream: bool = False,
     ) -> dict[str, Any] | Generator[dict[str, Any], None, None]:
         # ... existing setup code ...
         
-        # Convert messages to Ollama format
-        prompt = self._messages_to_prompt(messages)
-
         try:
             response = requests.post(
-                f"{self.base_url}/api/generate",
+                f"{self.base_url}/api/chat",
                 json={
                     "model": model,
-                    "prompt": prompt,
-                    "temperature": temperature,
+                    "messages": messages,  # Native OpenAI-compatible format
                     "options": {
+                        "temperature": temperature,
                         "num_predict": max_tokens,
                     },
                     "stream": stream,
                 },

This uses Ollama's /api/chat endpoint which natively understands OpenAI-style message formats.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

tests/test_ollama_integration.py (2)

41-58: Fix test structure and mock configuration.

The issues identified in past reviews remain valid:

1. Move from requests.exceptions import ConnectionError to top-level imports (line 54)
2. Reuse the same provider instance instead of creating provider2 (line 57)
3. The mock configuration with side_effect needs proper handling for both test cases

---

163-190: Fix mock configuration for router integration test.

The past review correctly identifies that the mock setup doesn't match how the router actually uses the Ollama provider. The response will contain a MagicMock instead of the expected string content, causing test failures.

scripts/setup_ollama.py (1)

203-205: Fix missing f-string prefix.

Line 204 is missing the f prefix for the f-string, so {model_name} won't be interpolated.

🔎 Proposed fix

     except subprocess.TimeoutExpired:
-        logger.warning("⚠️  Model download timed out - try again with: ollama pull {model_name}")
+        logger.warning(f"⚠️  Model download timed out - try again with: ollama pull {model_name}")
         return False

cortex/providers/ollama_provider.py (1)

122-130: Critical: Use Popen instead of subprocess.run for background service.

Line 125-128 uses subprocess.run() which blocks indefinitely waiting for ollama serve to complete. This should use subprocess.Popen() to start the service in the background, matching the pattern used in start_service() at line 173.

🔎 Proposed fix

             if install_result.returncode == 0:
                 logger.info("✅ Ollama installed successfully")
                 # Start Ollama service
-                subprocess.run(["ollama", "serve"],
-                             stdout=subprocess.DEVNULL,
-                             stderr=subprocess.DEVNULL,
-                             start_new_session=True)
+                subprocess.Popen(
+                    ["ollama", "serve"],
+                    stdout=subprocess.DEVNULL,
+                    stderr=subprocess.DEVNULL,
+                    start_new_session=True,
+                )
                 time.sleep(2)  # Give service time to start
                 return True

setup.py (2)

22-32: Consider skipping Ollama setup in development mode.

Running the Ollama setup script on every develop install could be disruptive during iterative development. Consider checking for a skip environment variable or detecting virtual environments to avoid repeated setup attempts.

🔎 Proposed improvement

 class PostDevelopCommand(develop):
     """Post-development setup for Ollama."""
     def run(self):
         develop.run(self)
+        # Skip in development if already configured
+        if os.getenv("CORTEX_SKIP_OLLAMA_SETUP"):
+            print("ℹ️  Skipping Ollama setup (CORTEX_SKIP_OLLAMA_SETUP is set)")
+            return
         # Run Ollama setup after development install
         try:

---

51-51: Consider updating minimum dependency versions.

The fallback requirements specify anthropic>=0.18.0 and openai>=1.0.0, which are quite old (2+ years). While no active CVEs were found, using more recent stable versions (e.g., anthropic>=0.75.0, openai>=2.14.0) would improve compatibility and maintainability. However, this is marked as "Chill" review, so this is optional unless you're concerned about long-term maintenance.

scripts/setup_ollama.py (1)

37-76: Consider adding security verification for installation script.

The function downloads and executes Ollama's installation script without verification. While this matches Ollama's official documentation, consider adding a note about security or optionally supporting SHA256 verification of the downloaded package for enhanced security posture.

cortex/providers/ollama_provider.py (1)

100-137: Security consideration: Installation script executed without verification.

The install_ollama() method downloads and directly executes Ollama's installation script without checksum verification. While this follows Ollama's official docs, consider documenting this security consideration in the method's docstring or adding optional SHA256 verification against published release checksums for enhanced security posture.

Configuration used: defaults

Review profile: CHILL

Plan: Pro